Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files          12       12           
  Lines        1281     1281           
=======================================
  Hits         1275     1275           
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be57ff7...53d5eaf. Read the comment docs.

There should not be any need to sleep. As soon as insert returns you should be able to read the latest data if you are using strong reads.

I've re-opened this, because I can't think of another solution. We're still seeing this flakiness-- the query sometimes returns the row inserted in the previous test.

I feel bad about adding timeouts to tests. They will hide real problems and will not actually make the test more stable.

Can we make this change in a branch (not in a forked repo), then remove the branch filter from CircleCI config in this branch to make it run system tests, and make several CI runs, I would say 10, to verify there are no flaky errors? If it really helps, get back to @vkedia to understand the reason.

@vkedia is this a strong read? If not, your comment leads me to believe that a latency is expected between data insertion and querying availability. But, what I don't know, is if this is a strong read or eventual.

I feel bad about adding timeouts to tests. They will hide real problems and will not actually make the test more stable.

I don't mind a timeout to give the server the time it needs to handle the operation. That's unavoidable. An alternative approach is polling, but if 5 seconds is always long enough, then polling would just add complication to the tests.

In either case, I'd like @vkedia's input first.

Friendly ping on this - system tests are still flaky and it makes me so sad :(

cc @snehashah16

I agree with vkedia's input, there is no reason to add this sleep.
Setting Strong in BeginTxn here

I am Node - illiterate, and therefore can provide a java example so that this works:

DatabaseClient client = Spanner.... 
// Strong bound is default. Though u can explicitly put it in your test to verify functionality
ReadOnlyTransaction txn = client.singleUseReadOnlyTransaction(); 
ResultSet resultSet = txn.read(table, KeySet.singleKey(key), columns);
assertThat(txn.getReadTimestamp()).isAtLeast(expected.timestamp);

@stephenplusplus - if this does not work, I assure u its an issue in the Node library code, and please dig into the root cause.
This has been extensively tested on Spanner and should work as intended.

Thanks @snehashah16. The tests were previously not running as part of a transaction. I switched to using transactions, but I'm not sure if that's what we want here (@callmehiphop ?).

@stephenplusplus I could be wrong, but I think that any calls made by a Table object get ran as a single-use transaction, so I don't think we need to explicitly run that as a transaction?

@callmehiphop Ah, thanks for the tip. Now, I just pass the transaction options to the database.run/runStream call directly. That seems like it will still enforce the transaction as a strong read.

@callmehiphop - could u please double check, single-use txn should use Strong Bound by default.

@stephenplusplus -
this should also work with singleUse, so please add appropriate tests.

Here's the code for database.runStream:

Database.prototype.runStream = function(query, options) {
  var self = this;

  if (is.string(query)) {
    query = {
      sql: query,
    };
  }

  var reqOpts = codec.encodeQuery(query);

  if (options) {
    reqOpts.transaction = {
      singleUse: {
        readOnly: TransactionRequest.formatTimestampOptions_(options),
      },
    };
  }

  delete reqOpts.json;
  delete reqOpts.jsonOptions;

  function makeRequest(resumeToken) {
    return self.pool_.requestStream({
      client: 'SpannerClient',
      method: 'executeStreamingSql',
      reqOpts: extend(reqOpts, {resumeToken}),
    });
  }

  return new PartialResultStream(makeRequest, {
    json: query.json,
    jsonOptions: query.jsonOptions,
  });
};

It looks like it's only a single use transaction if options are passed? And strong is not set by default, but it sounds like we need to do that?

@stephenplusplus I believe that is correct

@snehashah16 outside of a transaction, should we always use a single-use transaction? And if so, should we default to strong? Any other comments about how our default behavior should work are welcome.

ping @snehashah16 about the question above. Our method, database.run('SELECT ...') by default, does not create a transaction (single-use or otherwise). A user can choose to run in a single-use transaction by passing an options object to the method. Should we opt the user into a single-use transaction by default? If so, should we set strong reads by default?

I believe the fact that our code was not doing this contributed to why the delay proposed in this PR was required to receive accurate query results.

@stephenplusplus - Java always has strong reads for singleUse()

public ReadContext singleUse() { return singleUse(TimestampBound.strong()); }

Should we opt the user into a single-use transaction by default?

Yes, my recommendation

Can we merge here?

@stephenplusplus let me know if I shouldn't have done that 🤣